Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CTFE Machine: do not expose Allocation #85472

Merged
merged 1 commit into from
May 20, 2021
Merged

CTFE Machine: do not expose Allocation #85472

merged 1 commit into from
May 20, 2021
+11 −9

Conversation

RalfJung
Copy link
Member

Memory is careful now to not expose direct access to Allocation, but this one slipped through.
r? @oli-obk

@rust-highfive
Copy link
Collaborator

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 19, 2021
@oli-obk
Copy link
Contributor

oli-obk commented May 19, 2021

It would be fairly easy to write a lint that ensures that there are no Allocations in public parts of memory.rs or in the Machine trait. Is this something you think could be useful?

@RalfJung
Copy link
Member Author

I guess? Honestly I am not sure if it's worth it, it's also not that hard to audit this manually.^^

What I am much more worried about is code inside memory.rs using get_raw(_mut) and not doing bounds checks or not calling the access hooks...

@oli-obk
Copy link
Contributor

oli-obk commented May 19, 2021

OK, no lint then.

About the raw functions. They could return a wrapper type that requires you to properly do checks before getting access to things. Not sure if that is possible in all cases, but we could always have the checks return token types that the actual access consumes, paired with long-named constructors for these token types so you can Token::i_promise_i_am_up_to_no_good() if you do in fact need to skip the check

@oli-obk
Copy link
Contributor

oli-obk commented May 19, 2021

Anyway, this PR is good

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 19, 2021

📌 Commit 50a9f00 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 19, 2021
@RalfJung
Copy link
Member Author

They could return a wrapper type that requires you to properly do checks before getting access to things.

We already have those wrapper types, they are called AllocRef and AllocRefMut. ;)

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 19, 2021
@Dylan-DPC
Rollup merge of rust-lang#85472 - RalfJung:allocation, r=oli-obk
21007ee 
CTFE Machine: do not expose Allocation

`Memory` is careful now to not expose direct access to `Allocation`, but this one slipped through.
r? `@oli-obk`
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request May 19, 2021
Closed
@RalfJung RalfJung mentioned this pull request May 19, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 20, 2021
@bors
Auto merge of rust-lang#85486 - RalfJung:rollup-4ibcxuu, r=RalfJung
35bab92 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#84717 (impl FromStr for proc_macro::Literal)
 - rust-lang#85169 (Add method-toggle to <details> for methods)
 - rust-lang#85287 (Expose `Concurrent` (private type in public i'face))
 - rust-lang#85315 (adding time complexity for partition_in_place iter method)
 - rust-lang#85439 (Add diagnostic item to `CStr`)
 - rust-lang#85464 (Fix UB in documented example for `ptr::swap`)
 - rust-lang#85470 (Fix invalid CSS rules for a:hover)
 - rust-lang#85472 (CTFE Machine: do not expose Allocation)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9fa15ff into rust-lang:master May 20, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 20, 2021
@RalfJung RalfJung deleted the allocation branch May 20, 2021 20:05
@RalfJung RalfJung mentioned this pull request Jul 12, 2021
Merged
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jul 12, 2021
@JohnTitor
Rollup merge of rust-lang#87089 - RalfJung:ctfe-memory-cleanup, r=oli…
e46b790 
…-obk

CTFE engine: small cleanups

I noticed these while preparing a large PR, and figured I'd better send them ahead to not muddy the diff unnecessarily.

- remove remaining use of Pointer in Allocation API (I missed those in rust-lang#85472)
- remove unnecessary deallocate_local hack (this logic does not seem necessary any more)

r? `@oli-obk`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@oli-obk oli-obk

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.54.0
Development

Successfully merging this pull request may close these issues.
5 participants
@RalfJung @rust-highfive @oli-obk @bors @rustbot